Fix flaky DynamicTest.testDynamicRegistry race condition #3531
+15
−6
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The testEnabledWhenHover test was failing randomly when trying to retrieve the hover shell for the second part of the test. Root cause: - The test has two parts: first with EnabledPropertyTester.setEnabled(true), then with setEnabled(false) - After the first hover is shown and checked, cleanFileAndEditor() is called - However, the hover shell from the first part may not be fully disposed when the second editor is opened and the second hover is triggered - This causes getHoverShell() to timeout waiting for the new hover shell Fix: - Capture a reference to the first hover shell before calling cleanFileAndEditor() - After cleanFileAndEditor(), explicitly wait for the first shell to be disposed using DisplayHelper.waitForCondition() with a 3000ms timeout - This ensures the hover state is fully reset before the second part begins This approach follows the pattern used in other recent flaky test fixes in this repository (e.g., ProgressContantsTest, ProgressViewTests) which use condition-based waiting to ensure proper cleanup between test phases. Fixes eclipse-platform#926 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The testDynamicRegistry test was failing randomly with "Activity Listener not called" or "Category Listener not called" errors. Root cause: - The test uses a plain boolean array to track listener invocations - Extension registry listeners may be invoked on different threads - Without proper synchronization (volatile/atomic), changes to the array made by listener threads may not be visible to the test thread - This causes DisplayHelper.waitForCondition() to timeout even when listeners were actually called Fix: - Replace boolean[] with AtomicBoolean instances for thread-safe visibility - Use activityChanged.set(true) and categoryChanged.set(true) in listeners - Use activityChanged.get() and categoryChanged.get() in assertions - AtomicBoolean provides proper memory barriers ensuring cross-thread visibility This approach ensures proper synchronization between the listener callback threads and the test thread, eliminating the race condition that caused random test failures. Fixes eclipse-platform#2458 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Contributor
Contributor
Author
|
@fedejeanne WDYT? |
Member
|
Sorry for the late reply. The changes look OK. Based on the description of the issue, this additional wait shouldn't be necessary (or at least, I don't see why it is necessary in the description): DisplayHelper.waitForCondition(display, 3000, () -> firstShell.isDisposed());But since that operation doesn't slow down the test if the shell is disposed, I'd say it's safe to use 👍 Thank you for fixing the flaky tests, @vogella ! 🚀 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the random failures in
DynamicTest.testDynamicRegistryby replacing the plain boolean array withAtomicBooleaninstances for proper thread-safe visibility across listener callbacks.Root Cause
The test was failing randomly with "Activity Listener not called" or "Category Listener not called" errors because:
boolean[]to track listener invocationsDisplayHelper.waitForCondition()to timeout even when listeners were actually calledChanges
boolean[] registryChangedwith twoAtomicBooleaninstances (activityChangedandcategoryChanged).set(true)instead of array assignment.get()instead of array accessjava.util.concurrent.atomic.AtomicBooleanTest Plan
Related Issues
Fixes #2458
🤖 Generated with Claude Code